Stop using distutils #2564
Stop using distutils #2564
Conversation
|
Thanks for the pr, care to keep 2.7 compat? |
|
Based on https://docs.python.org/2.7/library/sysconfig.html , it is 2.7 compatible? The CI fail looks unrelated: |
You changed the search paths for includes, i guess sysconfig and distutils.sysconfig does not return the expected paths on python 2.7 |
There was a problem hiding this comment.
It looks like you're picking the wrong path for the second call, but I think there's something wrong (different) with the include paths that are built and the actual system. I printed the difference between the two methods here https://github.com/unbit/uwsgi/actions/runs/6398069839/job/17367389012?pr=2567#step:8:13 and you can see that the newer call returns a /usr/local/ path for the includes.
|
I clearly latched onto platform-specific files in the docs, without considering it was for header files, nice catch! |
distutils is due to be removed in Python 3.12, but even so we don't need to use it, since the sysconfig module can handle our use-cases just fine for all supported Python versions.
s-t-e-v-e-n-k
force-pushed
the
no-more-distutils
branch
from
2783e8b
to
8041fc2
Compare
There was a problem hiding this comment.
It looks like debian has the following patch https://salsa.debian.org/cpython-team/python2/-/blob/2.7.18-12/debian/patches/distutils-install-layout.diff which is breaking this. You could specify PYTHONUSERBASE=1 to bypass the bug, but this looks like it will break other users if you did that to pass the tests.
You could handle it with the following suggestion (I've tested it with #2567), which you can pull into your branch by creating PR s-t-e-v-e-n-k/uwsgi@no-more-distutils...terencehonles:uwsgi:pr-2564-patch and merging it (which will update this PR).
However, this may break on Python 3.12 (which currently doesn't build) because if that patch still applies to Python 3, distutils is no longer included, and the path is still wrong, the include path will not be set correctly.
| @@ -1,10 +1,10 @@ | |||
| from distutils import sysconfig | |||
| import sysconfig | |||
There was a problem hiding this comment.
| import sysconfig | |
| import os | |
| import sysconfig | |
| def get_includes(): | |
| try: | |
| from distutils import sysconfig as legacy | |
| except ImportError: | |
| legacy = None | |
| yield sysconfig.get_path('include') | |
| yield sysconfig.get_path('platinclude') | |
| if legacy: | |
| yield legacy.get_python_inc() | |
| yield legacy.get_python_inc(plat_specific=True) |
There was a problem hiding this comment.
sysconfig.get_path also allows us to specify which scheme we want with the second argument, rather than what you've done here. However, I'm loathe to check if 'deb_system' is a suitable scheme, but this only appears to rear it's head on Python 2, rather than every version of Python 3 that also ships distutils.
barriere% python3
Python 3.9.2 (default, Feb 28 2021, 17:03:44)
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_path('include')
'/usr/include/python3.9'
>>>
barriere% python2
Python 2.7.18 (default, Jul 14 2021, 08:11:37)
[GCC 10.2.1 20210110] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sysconfig
>>> sysconfig.get_path('include')
'/usr/local/include/python2.7'
>>> sysconfig.get_path('include', 'deb_system')
'/usr/include/python2.7'
| '-I' + sysconfig.get_path('include'), | ||
| '-I' + sysconfig.get_path('platinclude') |
There was a problem hiding this comment.
| '-I' + sysconfig.get_path('include'), | |
| '-I' + sysconfig.get_path('platinclude') | |
| '-I' + i for i in filter(os.path.exists, get_includes()), |
|
Merged a modified version in #2570 |
distutils is due to be removed in Python 3.12, but even so we don't need to use it, since the sysconfig module can handle our use-cases just fine for all supported Python versions.